-
Notifications
You must be signed in to change notification settings - Fork 5.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
planner/core: get back range columns pruning after last refactor #15169
Conversation
Codecov Report
@@ Coverage Diff @@
## master #15169 +/- ##
================================================
- Coverage 80.3669% 80.2766% -0.0903%
================================================
Files 502 503 +1
Lines 133805 132280 -1525
================================================
- Hits 107535 106190 -1345
+ Misses 17823 17700 -123
+ Partials 8447 8390 -57 |
col, fn, err = makePartitionByFnCol(ds.ctx, ds.TblCols, ds.names, pi.Columns[0].L) | ||
// Partition by range columns. | ||
if len(pi.Columns) > 0 { | ||
return s.pruneRangeColumnsPartition(ds, pi) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we let "range column by int" like this can use makeLessThanData
?
create table t (a int) partition by range columns (a) (
partition p0 values less than (0),
partition p1 values less than (500),
partition p2 values less than (1000),
partition p3 values less than (1500),
partition p4 values less than (2000)
);
select * from t;
(that way should be faster?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's faster, but the case is not general, we have to write more code for this optimizing
} | ||
} | ||
var expr expression.Expression | ||
expr, err = expression.NewFunction(sctx, op, types.NewFieldType(mysql.TypeLonglong), p.data[ith], v) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both RANGE COLUMNS partitioning and LIST COLUMNS partitioning support the use of non-integer columns for defining value ranges or list members. The permitted data types are shown in the following list:
All integer types: TINYINT, SMALLINT, MEDIUMINT, INT (INTEGER), and BIGINT. (This is the same as with partitioning by RANGE and LIST.) Other numeric data types (such as DECIMAL or FLOAT) are not supported as partitioning columns.
DATE and DATETIME. Columns using other data types relating to dates or times are not supported as partitioning columns.
The following string types: CHAR, VARCHAR, BINARY, and VARBINARY. TEXT and BLOB columns are not supported as partitioning columns.
it seems partition by range columns
only support three type int/date/string, so ideal way is impl two addition lessThanData type ---------- lessThanDataInt
, lessThanDataTime
and lessThanDataString
to void new & eval expression for each compare?
but LGTM to use current impl to quick fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If some type is not supported, we should prevent the case in DDL check, rather than in partition pruning.
lessThanDataTime
and lessThanDataString
seems good, we can optimize it later.
@winoros please help to take a look if free |
/rebuild |
@imtbkcat PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/merge |
/run-all-tests |
What problem does this PR solve?
What is changed and how it works?
Fix regression after #14834
Partition pruning for 'partition by range columns' table is broken after that refactor, as we only handle integer in the new partition pruning algorithm, 'by range columns' may contain non-integer data.
Now I've added expression comparing in addition to integer in this commit.
Check List
Tests